Add coverage target in Makefile for coverage reporting #71

shin- · 2018-05-01T01:42:03Z

No description provided.

mat007 · 2018-05-01T07:26:28Z

codecoverage/coverage-bin

+#!/bin/sh
+
+# This script is a proxy that injects the required test flags and strips out test output
+# It allows us to use a coverage-enabled binary for e2e tests


Do we need a script for this? Can't it be done from the Makefile?

Yes, we do; the script isn't called by the Makefile directly, but is invoked by the e2e test suite as if it was the actual docker-app binary.

More details on why we're doing things this way if you're curious:

https://www.elastic.co/blog/code-coverage-for-your-golang-system-tests

https://www.google.com/amp/s/blog.cloudflare.com/go-coverage-with-external-tests/amp/

I see, thanks.

mat007 · 2018-05-02T04:43:40Z

.gitignore

@@ -1,4 +1,5 @@
 *.tar.gz
 *.dockerapp
 _build/
+codecoverage/*.out


Could the output be moved into _build? This way all build intermediate files would be gathered in there.

Yeah, I can certainly do that.

mat007 · 2018-05-02T06:35:01Z

You seem to have pushed the coverage branch to docker/lunchbox instead of shin-/lunchbox, is it on purpose?

shin- · 2018-05-02T19:59:37Z

You seem to have pushed the coverage branch to docker/lunchbox instead of shin-/lunchbox, is it on purpose?

I got into the habit of doing that because the open source Jenkins would often ignore my PRs when submitted from my fork. I'll switch to shin-/lunchbox in the future.

shin- · 2018-05-03T00:30:13Z

Jenkinsfile

+                            sh 'make ci-coverage'
+                            archiveArtifacts 'cov/all.out'
+                            archiveArtifacts 'cov/coverage.html'
+                            sh 'curl -s https://codecov.io/bash | bash -s - -t 0b5323a7-aa90-4855-95ad-c859a917d611 -f cov/all.out -K'


I put the codecov token here for the time being but ideally this would be a build variable in Jenkins or something - @mat007 would you be able to help with this?

Sure, I'll have a look.

Ah I just realized this is actually uploading the reports to codecov…
So we don't really need to archive any artifacts nor copy files out of the container, do we?

It comes down to what people expect out of this and how we'd like to leverage the results. If we're okay with being reliant on codecov to produce our report then we can skip the archiving.

OTOH, I'm ok with the extra ~40kB and 2 seconds of processing if it gives us more options.

mat007 · 2018-05-03T04:47:23Z

Jenkinsfile

+                    steps {
+                        dir('src/github.com/docker/lunchbox') {
+                            checkout scm
+                            sh 'ls -la'


Left over from debugging?

Just saw the ones I left by mistake lines 68, 81 and 94, would you mind removing them as well?

mat007 · 2018-05-03T04:47:49Z

Jenkinsfile

+                            checkout scm
+                            sh 'ls -la'
+                            sh 'make ci-coverage'
+                            archiveArtifacts 'cov/all.out'


Do we need all.out as an artifact?

mat007 · 2018-05-03T15:49:01Z

Makefile

+ci-coverage:
+	docker build --target=build -t $(IMAGE_NAME)-cov:$(TAG) $(IMAGE_BUILD_ARGS) .
+	docker run --label $(COV_LABEL) $(IMAGE_NAME)-cov:$(TAG) make COMMIT=$(TAG) TAG=$(COMMIT) coverage
+	docker cp $$(docker ps -aql --filter label=$(COV_LABEL)):$(PKG_PATH)/_build/cov/ ./cov


The ./cov creates a folder out of _build when running make ci-coverage locally (I know we're not really supposed to but it proves useful as a poor man's building environment which only requires docker and make).
I suppose it all depends on what we want to do with the coverage reports in the end, a tar.gz might not be very useful…
Currently the generated HTML does not display well as an artifact on Jenkins, see https://ci.baguette.docker.paris/job/lunchbox/view/change-requests/job/PR-71/

Yeah, one would have to download the HTML file to view it correctly I imagine.

mat007 · 2018-05-03T16:27:27Z

Jenkinsfile

+                    agent {
+                        label 'gcp-linux-worker-0'
+                    }
+                    steps {


Right, if I have the secret configured correctly on the Jenkins side, here you only need to add

environment { CODECOV_TOKEN = credentials('jenkins-codeconv-token') }

and then remove the -t flag below and that should be it!

Secret configuration page for lunchbox on Jenkins: https://ci.baguette.docker.paris/job/lunchbox/credentials/store/folder/domain/_/

(Side note: It looks like I'm not authorized to access that page)

shin- · 2018-05-03T18:12:08Z

To tie up the threads on the artifacts discussion: I kept things broad for the time being so we can weigh our options. If we feel some are redundant or seldom useful, I'm happy to remove them.

mat007 · 2018-05-03T19:28:04Z

Jenkinsfile

@@ -44,7 +44,7 @@ pipeline {
            parallel {
                stage("Coverage report") {
                    environment {
-                        CODECOV_TOKEN = credentials('jenkins-codecov-token')
+                        CODECOV_TOKEN = credentials('jenkins-codeconv-token')


Woops, sorry, I'll fix that right away…

OK, fixed, you can remove the last commit, sorry about that!

mat007 · 2018-05-03T19:36:10Z

@chris-crone any opinion about how we might want to use the coverage report data?

shin- · 2018-05-29T20:50:19Z

Rebased + fixed some issues with the script uncovered by the new e2e tests

codecov · 2018-05-29T21:12:57Z

Codecov Report

❗ No coverage uploaded for pull request base (master@133bfaa). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #71   +/-   ##
=========================================
  Coverage          ?   39.57%           
=========================================
  Files             ?       40           
  Lines             ?     2332           
  Branches          ?        0           
=========================================
  Hits              ?      923           
  Misses            ?     1248           
  Partials          ?      161

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 133bfaa...b205e55. Read the comment docs.

vdemeester · 2018-05-30T07:29:39Z

Something to take into account on https://codecov.io/gh/docker/lunchbox/pull/71/tree though, pretty sure it doesn't cover any packages that has no tests (so 27% is only on packages that have some tests).

mat007

Vendoring seems to have issues?

mat007 · 2018-05-31T08:03:26Z

Jenkinsfile

+                    environment {
+                        CODECOV_TOKEN = credentials('jenkins-codecov-token')
+                    }
+


nit: extra blank line

mat007 · 2018-05-31T08:04:09Z

Makefile

+	@echo "Running e2e tests (coverage)..."
+	DOCKERAPP_BINARY=../e2e/coverage-bin $(GO_TEST) -v ./e2e
+	@echo "Running unit tests (coverage)..."
+	$(GO_TEST) -cover -test.coverprofile=_build/cov/unit.out $(shell go list ./... | grep -vE '/vendor/|/e2e')


nit: vendor is actually not listed by go list by default

mat007 · 2018-05-31T08:06:35Z

vendor/github.com/docker/yatee/yatee/yatee.go

@@ -20,7 +20,7 @@ type options struct {
 }

 // flatten flattens a structure: foo.bar.baz -> 'foo.bar.baz'
-func flatten(in map[string]interface{}, out map[string]interface{}, prefix string) {
+func flatten(in map[string]interface{}, out map[string]string, prefix string) {


Not sure this was intended to change yatee?

It appears that vendoring was broken on master. @mnottale is fixing it after which @shin- may need to rebase

mat007 · 2018-05-31T08:08:38Z

vendor/golang.org/x/tools/third_party/webcomponents/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2015 The Polymer Authors. All rights reserved.


What version of dep are you using? There seems to be a lot of license files being added here.

The latest, AFAICT:

$ dep version dep: version : v0.4.1 build date : 2018-01-24 git hash : 37d9ea0a go version : go1.9.1 go compiler : gc platform : linux/amd64

chris-crone

Generally LGTM, thanks @shin-!

chris-crone · 2018-05-31T08:02:15Z

Dockerfile

-  git
+  git \
+  util-linux \
+  bash


nit: I like to sort these alphabetically so that it's easier to see if something is installed or not

chris-crone · 2018-05-31T08:06:05Z

Gopkg.toml

+[[override]]
+  name = "github.com/spf13/pflag"
+  branch = "master"
+  source = "github.com/shin-/pflag"


would be good to upstream your changes here so that we don't need to rely on a fork

PR submitted: spf13/pflag#169
I'll keep an eye on it and update if anything happens.

chris-crone · 2018-05-31T08:50:54Z

vendor/github.com/docker/yatee/yatee/yatee.go

@@ -20,7 +20,7 @@ type options struct {
 }

 // flatten flattens a structure: foo.bar.baz -> 'foo.bar.baz'
-func flatten(in map[string]interface{}, out map[string]interface{}, prefix string) {
+func flatten(in map[string]interface{}, out map[string]string, prefix string) {


It appears that vendoring was broken on master. @mnottale is fixing it after which @shin- may need to rebase

chris-crone · 2018-05-31T09:25:07Z

@shin- I think you need to rebase on master and then run dep ensure --update github.com/docker/yatee

mnottale · 2018-05-31T09:37:47Z

No need for the dep ensure, a rebase should take care of that since the operation was done on master.

Signed-off-by: Joffrey F <[email protected]>

shin- · 2018-06-01T02:41:53Z

Updated, PTAL!

mat007 reviewed May 1, 2018

View reviewed changes

mat007 reviewed May 2, 2018

View reviewed changes

shin- force-pushed the coverage branch 13 times, most recently from 9b4460d to be08a48 Compare May 3, 2018 00:18

shin- commented May 3, 2018

View reviewed changes

mat007 reviewed May 3, 2018

View reviewed changes

shin- force-pushed the coverage branch from df94a8a to e7a4b57 Compare May 3, 2018 18:33

mat007 reviewed May 3, 2018

View reviewed changes

shin- force-pushed the coverage branch 2 times, most recently from d7ba3a8 to dfa4d2e Compare May 3, 2018 19:34

mat007 approved these changes May 3, 2018

View reviewed changes

shin- changed the title ~~[WIP] Add coverage target in Makefile for coverage reporting~~ Add coverage target in Makefile for coverage reporting May 3, 2018

shin- force-pushed the coverage branch from dfa4d2e to 4c06db1 Compare May 29, 2018 20:46

shin- force-pushed the coverage branch from 4c06db1 to f5450fe Compare May 29, 2018 21:12

mat007 suggested changes May 31, 2018

View reviewed changes

chris-crone reviewed May 31, 2018

View reviewed changes

shin- added 12 commits May 31, 2018 18:48

Add coverage target in Makefile for coverage reporting

6cb629a

Signed-off-by: Joffrey F <[email protected]>

Vendor gocovmerge tool

eff17e7

Signed-off-by: Joffrey F <[email protected]>

Scripts cleanup

eb11e4e

Signed-off-by: Joffrey F <[email protected]>

Vendor patched version of spf13/pflag

550fd77

Signed-off-by: Joffrey F <[email protected]>

Fix flag order

d1c85c2

Signed-off-by: Joffrey F <[email protected]>

Clean up directory structure

a9d5881

Signed-off-by: Joffrey F <[email protected]>

Run coverage in CI

c04818c

Signed-off-by: Joffrey F <[email protected]>

Cleanup

4136efb

Signed-off-by: Joffrey F <[email protected]>

Use codecov token from Jenkins credentials

abdac4c

Signed-off-by: Joffrey F <[email protected]>

Keep produced artifacts under _build/ in ci-coverage

9aad848

Signed-off-by: Joffrey F <[email protected]>

Fix coverage-bin script

8095120

Signed-off-by: Joffrey F <[email protected]>

cleanup

d4ddc16

Signed-off-by: Joffrey F <[email protected]>

shin- force-pushed the coverage branch from f5450fe to d4ddc16 Compare June 1, 2018 01:50

shin- added 2 commits May 31, 2018 19:06

Fix botched gocovmerge vendoring

e7d1ee8

Signed-off-by: Joffrey F <[email protected]>

Follow main package move

b205e55

Signed-off-by: Joffrey F <[email protected]>

shin- force-pushed the coverage branch from 2c28f69 to b205e55 Compare June 1, 2018 02:30

mat007 approved these changes Jun 1, 2018

View reviewed changes

chris-crone approved these changes Jun 1, 2018

View reviewed changes

chris-crone merged commit 9c4e622 into master Jun 1, 2018

vdemeester deleted the coverage branch June 1, 2018 10:23

		@@ -0,0 +1,27 @@
		Copyright (c) 2015 The Polymer Authors. All rights reserved.

Add coverage target in Makefile for coverage reporting #71

Add coverage target in Makefile for coverage reporting #71

Uh oh!

Conversation

shin- commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 commented May 2, 2018

Uh oh!

shin- commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shin- commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 commented May 3, 2018

Uh oh!

shin- commented May 29, 2018

Uh oh!

codecov bot commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vdemeester commented May 30, 2018

Uh oh!

mat007 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shin- commented May 1, 2018 •

edited

Loading

mat007 May 3, 2018 •

edited

Loading

mat007 May 3, 2018 •

edited

Loading

codecov bot commented May 29, 2018 •

edited

Loading